Skip to content

[dotnet] Revisit ignored intergration tests for chrome/edge#15324

Merged
nvborisenko merged 18 commits intoSeleniumHQ:trunkfrom
nvborisenko:dotnet-revisit-ignored-tests
Feb 24, 2025
Merged

[dotnet] Revisit ignored intergration tests for chrome/edge#15324
nvborisenko merged 18 commits intoSeleniumHQ:trunkfrom
nvborisenko:dotnet-revisit-ignored-tests

Conversation

@nvborisenko
Copy link
Member

@nvborisenko nvborisenko commented Feb 22, 2025 •
edited by qodo-code-review bot
Loading

User description

Motivation and Context

To keep actual expected behaviour.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • I have read the contributing document.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

PR Type

Tests

Description

  • Removed ignored browser annotations for Chrome and Edge in multiple test cases.

  • Revisited and enabled previously ignored integration tests for better coverage.

  • Adjusted test cases to ensure compatibility across browsers.

  • Improved test reliability by removing unnecessary platform-specific conditions.

Changes walkthrough 📝

Relevant files
Tests
ClearTest.cs
Re-enabled tests for disabled input elements                         

dotnet/test/common/ClearTest.cs

  • Removed ignored browser annotations for Chrome and Edge.
  • Enabled tests for disabled text input and text area behavior.
    		• +0/-4     
    ContentEditableTest.cs
    Re-enabled and adjusted content-editable element tests     

    dotnet/test/common/ContentEditableTest.cs

  • Removed ignored browser annotations for Chrome and Edge.
  • Enabled tests for content-editable elements with existing values.
  • Adjusted tests for TinyMCE and multi-text-node content-editable
    elements.
    		• +0/-6     
    CookieImplementationTest.cs
    Re-enabled cookie retrieval test in frames                             

    dotnet/test/common/CookieImplementationTest.cs

  • Removed ignored browser annotations for Chrome and Edge.
  • Enabled test for retrieving cookies in a frame.
    		• +0/-2     
    ElementFindingTest.cs
    Re-enabled invalid XPath handling tests                                   

    dotnet/test/common/ElementFindingTest.cs

  • Removed ignored browser annotations for Chrome and Edge.
  • Enabled tests for invalid XPath handling in element finding.
    		• +0/-16   
    ExecutingAsyncJavascriptTest.cs
    Re-enabled async script web element array test                     

    dotnet/test/common/ExecutingAsyncJavascriptTest.cs

  • Removed ignored browser annotation for Chrome.
  • Enabled test for returning arrays of web elements from async scripts.
    		• +0/-1     
    BasicMouseInterfaceTest.cs
    Simplified hover test logic                                                           

    dotnet/test/common/Interactions/BasicMouseInterfaceTest.cs

  • Removed unnecessary platform-specific condition for hover test.
  • Simplified test logic for hovering over elements.
    		• +0/-4     
    SvgDocumentTest.cs
    Re-enabled SVG element click test                                               

    dotnet/test/common/SvgDocumentTest.cs

  • Removed ignored browser annotations for Chrome and Edge.
  • Enabled test for clicking on SVG elements.
    		• +0/-2     
    UploadTest.cs
    Re-enabled file input click test                                                 

    dotnet/test/common/UploadTest.cs

  • Removed ignored browser annotations for Chrome and Edge.
  • Enabled test for clicking file input elements.
    		• +0/-2     
    Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
    •

    @qodo-code-review
    Copy link
    Contributor

    qodo-code-review bot commented Feb 22, 2025 •
    edited
    Loading

    PR Reviewer Guide 🔍

    (Review updated until commit 588f99b)

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Test Reliability

    Multiple tests still have browser-specific ignore annotations for Firefox and Safari. Consider investigating if these can also be enabled like Chrome/Edge were.

    [IgnoreBrowser(Browser.Firefox, "Driver prepends text in contentEditable areas")]
[IgnoreBrowser(Browser.Safari, "Driver prepends text to contentEditable areas")]
    Potential Bug

    Test for retrieving cookies in frames was previously ignored for Chrome/Edge due to known issues. Verify that the underlying issues have been fixed before enabling.

    [IgnoreBrowser(Browser.Firefox, "https://github.com/mozilla/geckodriver/issues/1104")]
public void GetCookiesInAFrame()
{
    driver.Url = EnvironmentManager.Instance.UrlBuilder.WhereIs("animals");

    @qodo-code-review
    Copy link
    Contributor

    qodo-code-review bot commented Feb 22, 2025 •
    edited
    Loading

    PR Code Suggestions ✨

    Latest suggestions up to 588f99b
    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Add native events support check

    Consider adding a check for native events support before running the hover test
    to ensure consistent behavior across platforms. The test might fail silently on
    platforms that don't support native events.

    dotnet/test/common/Interactions/BasicMouseInterfaceTest.cs [278-285]

     public void ShouldAllowUsersToHoverOverElements()
 {
+    if (!driver.IsNativeEventsEnabled)
+    {
+        Assert.Ignore("Skipping test: Hover requires native events support");
+    }
+    
     driver.Url = javascriptPage;
 
     IWebElement element = driver.FindElement(By.Id("menu1"));
 
     IWebElement item = driver.FindElement(By.Id("item1"));
     Assert.That(item.Text, Is.EqualTo(""));
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    __

    Why: The suggestion addresses a potential reliability issue by adding a crucial check for native events support, which is essential for hover functionality to work correctly across different platforms. Without this check, tests might fail silently on unsupported platforms.

    		Medium
    Learned
    best practice
    Add null checks for web elements returned from FindElement to prevent potential NullReferenceExceptions

    The test method should validate that the web elements element and item are not
    null before proceeding with the test, as FindElement can return null. Add
    ArgumentNullException checks after finding the elements.

    dotnet/test/common/BasicMouseInterfaceTest.cs [282-285]

     IWebElement element = driver.FindElement(By.Id("menu1"));
+ArgumentNullException.ThrowIfNull(element, nameof(element));
 
 IWebElement item = driver.FindElement(By.Id("item1"));
+ArgumentNullException.ThrowIfNull(item, nameof(item));
 Assert.That(item.Text, Is.EqualTo(""));

    [To ensure code accuracy, apply this suggestion manually]

    Suggestion importance[1-10]: 6
    		Low
    • More

    Previous suggestions

    Suggestions up to commit f4e4da5
    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Add hover support validation

    The platform check for native events was removed but no alternative validation
    was added. Consider adding a check to verify hover functionality is supported by
    the current browser/driver combination.

    dotnet/test/common/Interactions/BasicMouseInterfaceTest.cs [278-285]

     public void ShouldAllowUsersToHoverOverElements()
 {
     driver.Url = javascriptPage;
 
     IWebElement element = driver.FindElement(By.Id("menu1"));
+    
+    if (!TestUtilities.IsHoverSupported(driver))
+    {
+        Assert.Ignore("Hover functionality not supported by current browser/driver");
+    }
 
-    IWebElement item = driver.FindElement(By.Id("item1"));
+    IWebElement item = driver.FindElement(By.Id("item1")); 
     Assert.That(item.Text, Is.EqualTo(""));
    Suggestion importance[1-10]: 8

    __

    Why: The suggestion addresses a potential reliability issue by adding a necessary validation check for hover support after the platform-specific check was removed. This helps prevent test failures on unsupported configurations.

    		Medium

    @nvborisenko nvborisenko marked this pull request as draft February 23, 2025 09:00
    @nvborisenko nvborisenko requested a review from diemol February 23, 2025 22:53
    @nvborisenko nvborisenko marked this pull request as ready for review February 23, 2025 22:53
    @nvborisenko
    Copy link
    Member Author

    nvborisenko commented Feb 24, 2025

    Nice to see it executable again, thanks.

    @nvborisenko nvborisenko merged commit 9524fc8 into SeleniumHQ:trunk Feb 24, 2025
    10 checks passed
    sandeepsuryaprasad pushed a commit to sandeepsuryaprasad/selenium that referenced this pull request Mar 23, 2025
    @nvborisenko @sandeepsuryaprasad
    [dotnet] Revisit ignored intergration tests for chrome/edge (Selenium…
    bf1ad0c 
    …HQ#15324)
    @dependabot dependabot bot mentioned this pull request Sep 18, 2025
    Merged
    @dependabot dependabot bot mentioned this pull request Oct 1, 2025
    Closed
    @dependabot dependabot bot mentioned this pull request Oct 24, 2025
    Closed
    This was referenced Dec 21, 2025
    Open
    Closed
    Closed
    Closed
    This was referenced Jan 19, 2026
    Open
    Open
    Open
    Merged
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Reviewers

    @diemol diemol Awaiting requested review from diemol

    Assignees

    No one assigned

    Labels

    Review effort 2/5

    Projects

    None yet

    Milestone

    No milestone

    Development

    Successfully merging this pull request may close these issues.

    1 participant

    @nvborisenko